Skip to content

fix(ts): xtrim threshold accepts string#2828

Merged
nkaradzhov merged 2 commits intoredis:v5from
Eomm:patch-1
Aug 15, 2025
Merged

fix(ts): xtrim threshold accepts string#2828
nkaradzhov merged 2 commits intoredis:v5from
Eomm:patch-1

Conversation

@Eomm
Copy link
Copy Markdown
Contributor

@Eomm Eomm commented Sep 5, 2024 •
edited
Loading

Description

The XTRIM command accepts a streamId that is a string:

XTRIM foo MINID "1725522294258-0"

Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@leibale
Copy link
Copy Markdown
Contributor

leibale commented Sep 19, 2024

Good catch! Can you please do it for the v5 branch instead? Thanks!

@Eomm Eomm changed the base branch from master to v5 December 2, 2024 15:44
@nkaradzhov
Copy link
Copy Markdown
Collaborator

nkaradzhov commented Aug 5, 2025

Hi @Eomm, if you are still up to it, we can resurrect this PR and finish it

@Eomm
Copy link
Copy Markdown
Contributor Author

Eomm commented Aug 5, 2025

Hi @Eomm, if you are still up to it, we can resurrect this PR and finish it

I will try to recreate it by EOD 👍🏼

@Eomm Eomm marked this pull request as ready for review August 6, 2025 10:29
nkaradzhov
nkaradzhov reviewed Aug 13, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nkaradzhov nkaradzhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Eomm, sorry for late response. This looks good.
Just for clarity, i think its good to change all MINID tests to use string instead of number.

@Eomm
Copy link
Copy Markdown
Contributor Author

Eomm commented Aug 14, 2025

It seems there is only one test that uses the number:

node-redis/packages/client/lib/commands/XTRIM.spec.ts

Lines 18 to 19 in fceb609

parseArgs(XTRIM, 'key', 'MINID', 123),
['XTRIM', 'key', 'MINID', '123']

Would you like to change it or keep it to verify the number input compatibility?

@nkaradzhov
Copy link
Copy Markdown
Collaborator

nkaradzhov commented Aug 15, 2025

I guess thats fine. Thanks for the contribution!

@nkaradzhov nkaradzhov merged commit 7d905c0 into redis:v5 Aug 15, 2025
nkaradzhov added a commit that referenced this pull request Aug 15, 2025
@nkaradzhov
Revert "fix(ts): xtrim threshold accepts string (#2828)"
75d6267 
This reverts commit 7d905c0.
@nkaradzhov
Copy link
Copy Markdown
Collaborator

nkaradzhov commented Aug 15, 2025

@Eomm sorry, i clicked "merge" without looking. This is supposed to be merged agains master, not v5. Could you reopen against master?

@Eomm
Copy link
Copy Markdown
Contributor Author

Eomm commented Aug 15, 2025

Done: #3058

I was sure I read that I had to target v5 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nkaradzhov nkaradzhov nkaradzhov left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Eomm @leibale @nkaradzhov